Skip to content

Conversation

@evgueni-ovtchinnikov
Copy link
Contributor

Changes in this pull request

Current C++ DataContainer classes use static casts of void* pointers, which may be unsafe (cf. #1193).

This PR implements an alternative template-based design whereby static casts are only used on C interface level, which is not supposed to be used by SIRF users.

Testing performed

Related issues

Fixes #1193

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have added docstrings/doxygen in line with the guidance in the developer guide
  • I have implemented unit tests that cover any new or modified functionality
  • The code builds and runs on my machine
  • CHANGES.md has been updated with any functionality change

Contribution Notes

Please read and adhere to the contribution guidelines.

Please tick the following:

  • The content of this Pull Request (the Contribution) is intentionally submitted for inclusion in SIRF (the Work) under the terms and conditions of the Apache-2.0 License.

Copy link
Member

@KrisThielemans KrisThielemans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some probably mostly random comments but hopefully helpful in this review.

I'm not sure if templating ImageData is a good idea. It certainly breaks backwards compatibility in a huge way. Possibly this could be done by having an intermediate ImageDataTemplate like you did for DataContainer. However, it might not work, as I guess we'd then need to derive ImageDatafromDataContainer, but ImageDataTemplatefromDataContainerTemplate`. This can be done in C++, but has the potential for some confusion.

@KrisThielemans KrisThielemans added this to the 3.5 milestone Jun 8, 2023
@KrisThielemans KrisThielemans removed this from the 3.5 milestone Jun 22, 2023
@evgueni-ovtchinnikov
Copy link
Contributor Author

Left unattended for a long time, resulting in weird conflicts with master. Taken over by PR #1210.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Longer term solutions to DataContainer data type issue needed.

3 participants